Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[openxlsx] Add new port #41150

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from
Draft

Conversation

motazmuhammad
Copy link

@motazmuhammad motazmuhammad commented Sep 24, 2024

Fixes #40931

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • [x ] The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@motazmuhammad
Copy link
Author

@motazmuhammad please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@MonicaLiu0311
Copy link
Contributor

Why add CMakeLists.txt for libxml? It's already provided upstream.

@MonicaLiu0311
Copy link
Contributor

New port

  1. You can create the yalantinglibs folder in vcpkg/ports/ and add portfile.cmake and vcpkg.json:
vcpkg/ports/testport/
    portfile.cmake
    vcpkg.json

Then run ./vcpkg install testport, if the installation is successful, it proves that there is no problem with the files you added.

  1. Next, run ./vcpkg x-add-verison testport, the vcpkg/verisons/t-/testport.josn file will be added automatically, and a record will be added in vcpkg/verisons/baseline.json.
vcpkg/versions/
    t-/
        testport.josn (automatically generated)
    baseline.json (automatically add records)

The modification needs to meet the requirements: maintainer-guide

@MonicaLiu0311 MonicaLiu0311 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 25, 2024
@MonicaLiu0311 MonicaLiu0311 changed the title Open xlsx [openxlsx] Add new port Sep 26, 2024
ports/openxlsx/vcpkg.json Outdated Show resolved Hide resolved
ports/openxlsx/usage Outdated Show resolved Hide resolved
ports/openxlsx/portfile.cmake Outdated Show resolved Hide resolved
@MonicaLiu0311
Copy link
Contributor

New port

  1. You can create the yalantinglibs folder in vcpkg/ports/ and add portfile.cmake and vcpkg.json:
vcpkg/ports/testport/
    portfile.cmake
    vcpkg.json

Then run ./vcpkg install testport, if the installation is successful, it proves that there is no problem with the files you added.

  1. Next, run ./vcpkg x-add-verison testport, the vcpkg/verisons/t-/testport.josn file will be added automatically, and a record will be added in vcpkg/verisons/baseline.json.
vcpkg/versions/
    t-/
        testport.josn (automatically generated)
    baseline.json (automatically add records)

The modification needs to meet the requirements: maintainer-guide

Reminder: The version file is missing. Please refer to: 39493/files.

@motazmuhammad
Copy link
Author

@MonicaLiu0311 Thank you very much for all your insightful comments, I really appreciate them, I updated the fork with your suggested changes ( according to my best understanding). Please let me know if you have further comments, and please be patient with me).

Thank you again

@MonicaLiu0311
Copy link
Contributor

When testing usage, the following error occurs:

E:\test\build>cmake --build .
MSBuild version 17.11.2+c078802d4 for .NET Framework

  1>Checking Build System
  Building Custom Rule E:/test/CMakeLists.txt
  test.cpp
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.41.34120\include\variant(12): warning STL4038
: The contents of <variant> are available only with C++17 or later. [E:\test\build\main.vcxproj]
E:\openxlsx\installed\x64-windows\include\OpenXLSX\headers\XLXmlParser.hpp(50,10): error C1083: Cannot open include fil
e: 'external/pugixml/pugixml.hpp': No such file or directory [E:\test\build\main.vcxproj]
  (compiling source file '../test.cpp')
test.cpp
#include <iostream>
#include "OpenXLSX.hpp"

using namespace std;

int main()
{
cout << "Hello CMake." << endl;
return 0;
}

CMakeLists.txt
cmake_minimum_required (VERSION 3.8)

set(CMAKE_TOOLCHAIN_FILE "E:/openxlsx/scripts/buildsystems/vcpkg.cmake")

project ("test")

add_library (main "test.cpp")

find_package(OpenXLSX CONFIG REQUIRED)
target_link_libraries(main PRIVATE OpenXLSX::OpenXLSX)

@motazmuhammad
Copy link
Author

When testing usage, the following error occurs:

E:\test\build>cmake --build .
MSBuild version 17.11.2+c078802d4 for .NET Framework

  1>Checking Build System
  Building Custom Rule E:/test/CMakeLists.txt
  test.cpp
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.41.34120\include\variant(12): warning STL4038
: The contents of <variant> are available only with C++17 or later. [E:\test\build\main.vcxproj]
E:\openxlsx\installed\x64-windows\include\OpenXLSX\headers\XLXmlParser.hpp(50,10): error C1083: Cannot open include fil
e: 'external/pugixml/pugixml.hpp': No such file or directory [E:\test\build\main.vcxproj]
  (compiling source file '../test.cpp')

test.cpp
CMakeLists.txt

@MonicaLiu0311 Thank you very much that is fixed now. I have a question, if you dont mind please reply to them.

  1. Right now I am using the head revision (master branch) as evident by the portfile REF master
    is that ok, I tried using REF "${VERSION}" however, the correct file was not picked up unless i make it
    REF "${VERSION}"
    REF "refs/tags/v${VERSION}"
    and if I do that, I need to change the SHA512 , do I need to create a seperate SHA for each version? I could not figure this out from the documents.

Thank you again for your patience. I hope to become a regular contributor to vcpkg

@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Sep 30, 2024

REF "refs/tags/v${VERSION}"

The regular expression for ${VERSION} matches only VERSION semantics. The upstream released version is "v0.3.2" not "0.3.2".

Notes:
the patch file is to fix a compilation error in vs2019, 2022, this was already fixed in the master but not committed to the latest release
@motazmuhammad
Copy link
Author

@MonicaLiu0311 Thank you very much for your comments, I added the changes you recommended. I, now, have two new questions.

  1. Since I am using the v.0.3.2. now I had to add a patch for the code to compile on visual studio. This patch was actually later added to the master branch, but not to the latest release branch. Is that okay? or should the patch be added conditionally ( conditioned on the version number?)

  2. in the context of patching, this patch is not needed for the master release, therefore, the user would not be able to use the master branch via vcpkg, is that okay?

Thank you for your patience

@motazmuhammad
Copy link
Author

When testing usage, the following error occurs:

E:\test\build>cmake --build .
MSBuild version 17.11.2+c078802d4 for .NET Framework

  1>Checking Build System
  Building Custom Rule E:/test/CMakeLists.txt
  test.cpp
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.41.34120\include\variant(12): warning STL4038
: The contents of <variant> are available only with C++17 or later. [E:\test\build\main.vcxproj]
E:\openxlsx\installed\x64-windows\include\OpenXLSX\headers\XLXmlParser.hpp(50,10): error C1083: Cannot open include fil
e: 'external/pugixml/pugixml.hpp': No such file or directory [E:\test\build\main.vcxproj]
  (compiling source file '../test.cpp')

test.cpp
CMakeLists.txt

This now works, but please note that to be able to use openxlsx in the cmakelists.txt file one has to add the following line set(CMAKE_CXX_STANDARD 17) as the some of the header files are dependent on c++17, should that be mentioned in the usage file in one way or the other?

@dg0yt
Copy link
Contributor

dg0yt commented Oct 1, 2024

  1. Right now I am using the head revision (master branch) as evident by the portfile
    REF master

is that ok,

No. REF must identify a particular immutable release, tag or commit. master is a branch name, and the contents may change. (HEAD_REF may point to master.)

I tried using

   REF "${VERSION}" 

however, the correct file was not picked up unless i make it

  REF "refs/tags/v${VERSION}"

This indicates that the tags are named v<version>, and you should use

   REF "v${VERSION}"

and if I do that, I need to change the SHA512 , do I need to create a seperate SHA for each version?

Yes. The SHA512 is to verify the downloaded archive, and it is also used as an source asset cache key.

note that to be able to use openxlsx in the cmakelists.txt file one has to add the following line set(CMAKE_CXX_STANDARD 17) as the some of the header files are dependent on c++17, should that be mentioned in the usage file in one way or the other?

If there is exported CMake config, the C++17 requirement could be in the exported compile features.
https://cmake.org/cmake/help/latest/manual/cmake-compile-features.7.html#requiring-language-standards

SHA512 52205b394383d45c0fb16599ab96453d8a5b9b5cd596096848cc888f47565b2713d9edded06b2ecd7b67736622badf136e4b1becc57bfa5bbdcb1e063a347084
HEAD_REF master
PATCHES
compilation_fix.patch
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit-pick:

Suggested change
compilation_fix.patch
compilation_fix.patch

Comment on lines 11 to 18
set(CMAKE_CXX_STANDARD 17)

vcpkg_cmake_configure(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS
-DOPENXLSX_BUILD_TESTS=OFF
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
set(CMAKE_CXX_STANDARD 17)
vcpkg_cmake_configure(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS
-DOPENXLSX_BUILD_TESTS=OFF
)
vcpkg_cmake_configure(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
OPTIONS
-DCMAKE_CXX_STANDARD=17
-DOPENXLSX_BUILD_TESTS=OFF
)

Portfiles run in script mode. CMAKE_CXX_STANDARD has no effect.
https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#portfiles-are-run-in-script-mode


vcpkg_cmake_configure(
SOURCE_PATH ${SOURCE_PATH}
PREFER_NINJA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
PREFER_NINJA

Default.

OPTIONS
-DOPENXLSX_BUILD_TESTS=OFF
)
vcpkg_cmake_install(DISABLE_PARALLEL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why DISABLE_PARALLEL?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not needed, nice catch, thank you very much

ports/openxlsx/usage Outdated Show resolved Hide resolved
@motazmuhammad
Copy link
Author

@dg0yt thank you very much for your comments, I really appreciate all of them, I have replied to them to the best of my knowledge. Please accept my apologies for taking too much of your time, this is my first contribution to vcpkg

@motazmuhammad
Copy link
Author

@MonicaLiu0311 Thank you very much for all your help and comments, now that all tests have passed is the pull request ok now?

@motazmuhammad
Copy link
Author

@MonicaLiu0311 @dg0yt is there anything else I need to do?

@motazmuhammad motazmuhammad marked this pull request as ready for review October 6, 2024 14:50
MonicaLiu0311
MonicaLiu0311 previously approved these changes Oct 8, 2024
@MonicaLiu0311
Copy link
Contributor

The usage test passed on x64-windows (header files found):

openxlsx provides CMake targets:

  find_package(OpenXLSX CONFIG REQUIRED)
  target_link_libraries(main PRIVATE OpenXLSX::OpenXLSX)

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Oct 8, 2024
Copy link
Contributor

@JavierMatosD JavierMatosD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial Review:

It looks like this port is vendoring dependencies. All the ports dependencies should be explicitly listed in the dependency section of the vcpkg.json.

@@ -0,0 +1,30 @@
# Header-only library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Header-only library
# Header-only library
set(VCPKG_BUILD_TYPE release)

ports/openxlsx/portfile.cmake Outdated Show resolved Hide resolved
vcpkg_cmake_install()
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/OpenXLSX)
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")
file(COPY "${SOURCE_PATH}/OpenXLSX/external" DESTINATION "${CURRENT_PACKAGES_DIR}/include")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

External? It looks like this is vending zippy, pugixml, and nowide

Pleas see https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-vendored-dependencies

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JavierMatosD one of the dependencies (zippy) is not in vcpkg, should it be added to vcpkg, in a separate port as a requirement for this port?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Please open another PR adding zippy as a port as it is a requirement for this port.

ports/openxlsx/portfile.cmake Show resolved Hide resolved
{
XLQuery query(XLQueryType::QuerySheetVisibility);
- query.template setParam("sheetID", relationshipID());
+ query.setParam("sheetID", relationshipID());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this patch needed? Has it been reported upstream?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JavierMatosD Yes, it has been fixed in the upstream but it has not been put in the release refs yet, it is in the master (main ) branch, however.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this patch is already fixed upstream, can you download and apply the patch directly instead of checking it in?

@JavierMatosD JavierMatosD marked this pull request as draft October 8, 2024 21:24
Co-authored-by: Javier Matos Denizac <javier.matosd@gmail.com>
Co-authored-by: Javier Matos Denizac <javier.matosd@gmail.com>
@BillyONeal BillyONeal removed the info:reviewed Pull Request changes follow basic guidelines label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] OpenXLSX
5 participants